-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DX - Refactor some of the sidebar frame function names to clarify behavior, and fix sidebar-sub-frame console error #8475
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8475 +/- ##
==========================================
- Coverage 73.47% 73.41% -0.07%
==========================================
Files 1334 1352 +18
Lines 41259 41579 +320
Branches 7686 7803 +117
==========================================
+ Hits 30316 30526 +210
- Misses 10943 11053 +110 ☔ View full report in Codecov by Sentry. |
Playwright test results - MV2Details
Flaky testsedgeSetup › auth.setup.ts › authenticate Skipped testschrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration |
Playwright test resultsDetails
Flaky testsedge › tests/pageEditor/saveMod.spec.ts › can save a standalone trigger mod |
// in this specific case we don't want to be running the CoPilot Data | ||
// initialization on a nested frame anyway, so we'll just eat the error and | ||
// warn to console for now. | ||
const connectedTarget = await getConnectedTarget(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check before the call that this is coming from the top-level sidebar frame?
From a Dev Ex perspective, I'd at least recommend clarifying in the warning message that the warning is expected for sidebar subframes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can also run in a modal, right? What check exactly are you talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditionally approved - see comment on the scope of the try-catch and/or checking for top-frame status before making the call
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
…avior, and fix sidebar-sub-frame console error (#8475) * debug changes * make the code a bit more clear, and try-catch around the error related to sub-frames * only wrap the getConnectedTarget call in try catch * fix types * fix var --------- Co-authored-by: Ben Loe <[email protected]> Co-authored-by: Todd Schiller <[email protected]>
What does this PR do?
getConnectedTarget()
in a try-catch to avoid console errors when it's called in a sub-frame in the sidebarChecklist